SoftSignalBackend wrapping arbitrary callables#1280
Conversation
|
Great job! No new security vulnerabilities introduced in this pull requestCommunicate with Checkmarx by submitting a PR comment with @Checkmarx followed by one of the supported commands. Learn about the supported commands here. |
|
Looks good so far. Testing it locally with the pymmcore-plus package. It would have definetely saved me a lot of time. I'm guessing some customization (such as changing the |
|
This PR only addresses procedural device declaration, right? Is a declarative approach achievable somehow? |
Right, this addresses procedural only. Is a declarative approach even desirable in this case? Seems like you will likely need to write getters and setters anyway. |
| store[0] = config.velocity | ||
| return config.velocity | ||
|
|
||
| backend = SoftSignalBackend(float, setter=config_setter) |
There was a problem hiding this comment.
Hmm, is it a requirement for Signal.set to take a different datatype to its internal datatype? I know we do this for type coercion (take a string or a float, convert, store as float), but this example seems to promote this to requiring a different datatype. This will break typing...
There was a problem hiding this comment.
I don't think I quite understand. How does it promote requiring something?
If CallableSignalBackend.put calls any callable then it must be typed Any or perhaps T. This requires that Signal.set also be typed Any or T.
There was a problem hiding this comment.
Example:
sig = soft_signal_rw(float, setter=config_setter)
await sig.set(32.4) # pyright says ok, runtime says ok
value = await sig.get_value() # value is float
await sig.set(MotorConfig(...)) # pyright says bad, runtime says ok
value = await sig.get_value() # value is floatI don't think I want to promote a pattern where the set argument type is different from the value type
There was a problem hiding this comment.
@coretl What do you suggest then? If we say that the setter can only take one argument of the same type as the value carried by the function, then that strictly limits the types of operations we can reasonably support.
There was a problem hiding this comment.
I have viewed a few third-party libraries for device control, not enough to constitute as an expert on the matter, but I haven't found so far the situation where an entire data structure is passed down to a function controlling a device. I've seen more use case of APIs with multiple parameters, but in that case using functools.partial to bind other arguments should be sufficient.
@burkeds have you encountered these situations before?
There was a problem hiding this comment.
Good point, we probably need the concept of setpoint and reading:
- initial value goes to self._setpoint and self.reading
- put value goes through converter, then stored as self._setpoint
- setter called on self._setpoint, if it returns a value that goes in self.reading, otherwise call getter to get that value
There was a problem hiding this comment.
A possible issue. Currently, the value is not converted until self.set_value(...).
If we put self._setpoint = self.converter.write_value(value) in set_value that works provided there is not setter. If there is a setter then the converted write value would not be accurate to assign as self._setpoint because the value is not converted before it is passed to the setter.
Should we pass the converted write value to the setter?
def set_value(self, value: SignalDatatypeT):
"""Set the current value, alarm and timestamp."""
self.reading = Reading(
value=self.converter.write_value(value),
timestamp=time.time(),
alarm_severity=0,
)
if self.callback:
self.callback(self.reading)
async def put(self, value: Any) -> None:
write_value = self.initial_value if value is None else value
if self._setter is not None:
written_value = await maybe_await(self._setter(value))
if written_value is not None:
self.set_value(written_value)
elif self._getter is not None:
await self._update_value_from_getter()
else:
self.set_value(write_value)
else:
self.set_value(write_value)There was a problem hiding this comment.
Should we pass the converted write value to the
setter?
I think that's probably best
There was a problem hiding this comment.
Because the setter is possibly async, it can't go in set_value. That means to pass the converted value to the setter we need to convert the value twice, once in put and once in set_value. I didn't want to do that.
Instead, I thought we could pass the unconverted value to the setter (as I am sure a user would expect) and the setpoint is then recorded as the value passed to set_value.
There was a problem hiding this comment.
I'm not sure I agree, let's go with an example of a float signal with a setter:
- I call
signal.set("34.56") - First this converts the value to
34.56 - Then it calls
setter(34.56) - Imagine that
setterreturns34.6
In this example I would suggest that the setpoint is 34.56 and the value is 34.6. Would you agree?
If so then this becomes:
async def put(self, value: Any) -> None:
self._setpoint = self.initial_value if value is None else self.converter.write_value(value)
if self._setter is not None:
readback = await maybe_await(self._setter(value))
if readback is not None:
self.set_value(readback)
elif self._getter is not None:
await self._update_value_from_getter()
else:
self.set_value(self._setpoint)
else:
self.set_value(self._setpoint)
I think for declarative we should wait for the FastCS example. Passing setters and getters via a declarative approach gets messy fast... |
In my case, using pymmcore-plus, there is a class MMCamera(StandardReadable, MMCoreDevice)
...where core creation is delegated to |
|
@coretl For some reason, If we relax the equality in the assertion to |
I've noticed this on other PRs, I'll investigate... |
There was a problem hiding this comment.
Pull request overview
This PR extends SoftSignalBackend to support callable-backed signals (custom getter/setter functions) and optional polling while subscribed, enabling non-EPICS/Tango integrations without writing a full custom backend.
Changes:
- Added
getter,setter, andpoll_periodsupport toSoftSignalBackend, plus polling lifecycle tied to subscription callbacks. - Updated
soft_signal_rw/soft_signal_r_and_setterfactories to accept and forward these new options. - Added unit tests and documentation (how-to + ADR) covering callable-backed usage patterns.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/ophyd_async/core/_soft_signal_backend.py |
Implements getter/setter hooks and polling for SoftSignalBackend. |
src/ophyd_async/core/_signal.py |
Extends soft-signal factory functions to accept getter/setter/polling configuration. |
tests/unit_tests/core/test_soft_signal_backend.py |
Adds focused backend-level tests for getter/setter and polling behavior. |
tests/unit_tests/core/test_signal.py |
Adds factory-level tests ensuring the new kwargs work via Signal APIs. |
tests/system_tests_tango/test_tango_command.py |
Adjusts Tango device fixture naming (unrelated to soft signals). |
docs/how-to/how-to-use-soft-signals.md |
New how-to guide documenting pure soft signals vs callable-backed patterns. |
docs/explanations/decisions/0019-soft-signals-backed-by-arbitrary-callables.md |
New ADR describing the callable-backed soft-signal decision and rationale. |
docs/conf.py |
Updates Sphinx nitpick ignore list for new public type aliases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def set_value(self, value: SignalDatatypeT): | ||
| """Set the current value, alarm and timestamp.""" | ||
| setp = self.converter.write_value(value) | ||
| self._setpoint = setp | ||
| self.reading = Reading( | ||
| value=self.converter.write_value(value), | ||
| value=setp, | ||
| timestamp=time.time(), | ||
| alarm_severity=0, | ||
| ) |
| async def put(self, value: Any) -> None: | ||
| write_value = self.initial_value if value is None else value | ||
| self.set_value(write_value) | ||
| if self._setter is not None: | ||
| returned_by_setter = await maybe_await(self._setter(write_value)) | ||
| if returned_by_setter is not None: | ||
| self.set_value(returned_by_setter) | ||
| elif self._getter is not None: | ||
| await self._update_value_from_getter() | ||
| else: | ||
| self.set_value(write_value) | ||
| else: | ||
| self.set_value(write_value) |
| Setter = ( | ||
| Callable[[SignalDatatypeT | None], SignalDatatypeT | None] | ||
| | Callable[[SignalDatatypeT | None], Awaitable[SignalDatatypeT | None]] | ||
| | None | ||
| ) | ||
| Getter = Callable[[], SignalDatatypeT | Awaitable[SignalDatatypeT]] | ||
|
|
There was a problem hiding this comment.
Disagree, #1280 (comment) would pass the converted value to the setter
| getter: Getter[SignalDatatypeT] | None = None, | ||
| setter: Setter[Any] | None = None, | ||
| poll_period: float | None = None, |
| assert await backend.get_setpoint() == 5.0 | ||
| assert await backend.get_value() == 99.0 |
| status = soft_signal_rw(str) | ||
| def configure_subsystem(*args, **kwargs) -> None: | ||
| # Apply config... | ||
| await status.set("configured") | ||
| config_cmd = soft_command(configure_subsystem) | ||
| await config_cmd.execute(...) | ||
| current_status = await status.read() | ||
| ``` |
| # Split outputs into individual signals | ||
| temp_signal = soft_signal_rw(float, getter=lambda: run_diagnostics()["temperature"]) | ||
| pressure_signal = soft_signal_rw(float, getter=lambda: run_diagnostics()["pressure"]) | ||
| async def run_diagnostics() -> None: | ||
| temp, pressure = _diagnostics() | ||
| await temp_signal.set(temp) | ||
| await pressure_signal.set(pressure) | ||
| diagnostics_cmd = soft_command(run_diagnostics) | ||
| result = await diagnostics_cmd.execute() | ||
| temp = await temp_signal.read() | ||
| pressure = await pressure_signal.read() | ||
| ``` |
| **Key Takeaways**: | ||
| 1. **Prioritize `SoftSignalBackend` with callables** for simple, type-aligned read/write operations (Case A). | ||
| 2. **Combine `Command` + `Signal`** when types diverge or actions yield secondary results (Cases B/C). | ||
| 3. **Avoid overloading signals**: If a callable performs an action *and* returns data, model the action as a `Command` and the data as one or more `Signal`s. |
| ### **Design Choices** | ||
| - All three parameters are **optional**. If none are provided, behavior remains identical to the existing `SoftSignalBackend`. | ||
| - The internal `self._reading` store remains the **single source of truth**. The `getter` updates this store rather than bypassing it, preserving coherence for subscriptions and cached reads. | ||
| - The `put` method accepts `SignalDatatypeT` (the same type as the signal's stored value) to maintain type safety and consistency. | ||
| - Polling tasks are used for subscriptions, starting in `set_callback` and canceling when subscriptions end. | ||
| - `get_setpoint()` **does not invoke the `getter`**; it returns the last value written to the `setter` or the initial value of. | ||
|
|

Add getter, setter, and poll_period to SoftSignalBackend
Closes #1279
Motivation
Users working outside of EPICS/Tango — wrapping third-party Python APIs, calling analysis scripts, or interfacing with devices that have their own Python drivers — currently have no supported path to create signals without writing a full custom
SignalBackend. This was raised at the Bluesky community workshop and is a known friction point for smaller lab-based groups.Changes
Three new keyword-only parameters are added to
SoftSignalBackend:getter: Callable[[], T | Awaitable[T]]— called onget_valueandget_readingto fetch the current value from an external source. Whenpoll_periodis also set, called periodically while a subscription is active.setter: Callable[[Any], T | None | Awaitable[T | None]]— called onputin place of the default internal store update. May return the settled value the device actually reached; if it returnsNoneand a getter is configured, the getter is called immediately to refresh the cache.poll_period: float— interval in seconds at which the getter is called while a subscription is active. Requiresgetterto be set.All three parameters are optional. When none are provided, behaviour is identical to the existing
SoftSignalBackend, so there are no breaking changes.The internal
self.readingstore remains the single source of truth. The getter updates it rather than bypassing it, keeping subscriptions and cached reads coherent.Modified
soft_signal_rwandsoft_signal_r_and_setterto take setter, getter, and poll_period arguments.Design notes
putis typed asAnyto allow heterogeneous setter input types (e.g. a config class, a raw integer count, a command string) that differ from the signal'sSignalDatatypeT.set_callback, tied to subscription lifetime.get_setpointdeliberately does not call the getter — it returns the last requested value, not the live device readback.